💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a221bdaec9

Review: WIP(rewards): surface achievement token rewards and fix rewards page display

Read-only review of the full PR diff (24 files) against main. Frontend-only change: RewardsCommunityTab.tsx + tests, rewardsApi/types normalization for two new fields, i18n across all 14 locales, and E2E label updates.

Walkthrough

The change (a) removes the hard 8-badge cap so the progress-circle row matches the "{unlocked} of {total}" count, (b) surfaces per-achievement token rewards (rewardTokens / rewardRecurring) as an amber pill with compact formatting (500K, 2M, +…/mo), (c) shows locked-achievement progress labels, (d) splits the single stats footer into a Discord card and an Activity card (adding longest-streak + a streak-in-days label), and (e) recomputes the "roles assigned" ratio over assignable (unlocked + role-configured) achievements. It's marked WIP in the title but is not a draft, so CI gates apply. Overall the component logic and test coverage are solid; there is one CI-blocking i18n issue that will fail the required pnpm i18n:english:check gate.

Changed-files summary

Findings

🔴 Major (CI-blocking) — es and pt rewardTokens are byte-identical to English → i18n:english:check fails

app/src/lib/i18n/es.ts and app/src/lib/i18n/pt.ts both add:

'rewards.community.rewardTokens': '+{tokens} tokens',

This is byte-identical to the English value (app/src/lib/i18n/en.ts). scripts/i18n-find-english.ts flags any Latin-locale value where v === en[k] (unless allowlisted) and process.exit(1) on any hit — so the required pnpm i18n:english:check gate (per CLAUDE.md) will fail. es/pt are in LATIN_LOCALES, the value isn't isTechnical (+{tokens} tokens contains a real word after placeholder-stripping), and rewards.community.rewardTokens is not in INTENTIONAL_ENGLISH. (You already hit and fixed the same rule for discordDetails — this one was missed.)

Note rewardTokensMonthly is fine — es /mes and pt /mês differ from en /mo. Only the non-recurring key collides.

Fix — "tokens" is a legitimate loanword in es/pt, so allowlist the key alongside discordDetails in scripts/i18n-find-english.ts:

   "rewards.community.discordDetails", // "Discord" — brand/product name, same in every locale
+  "rewards.community.rewardTokens", // "tokens" — accepted loanword; identical in es/pt

(Alternatively translate them, e.g. es/pt +{tokens} fichas, but the loanword is the more natural choice for a token economy.) Please run pnpm i18n:english:check locally to confirm it passes.

Nitpicks / questions

• summary.assignedDiscordRoleCount is now dead for the UI. The component previously read snapshot.summary.assignedDiscordRoleCount; it now derives assignedRoleCount locally from achievements (RewardsCommunityTab.tsx:128). The backend field is still normalized (rewardsApi.ts:135) and asserted in tests but no longer rendered. Intended? If the backend count and the locally-derived count can diverge, worth a one-line comment on why the local derivation wins (consistency with the assignableRoleCount denominator). — minor / question
• Badge aria-label on a plain div. RewardsCommunityTab.tsx:349 puts aria-label={role?.title} on a non-interactive div with no role; screen-reader support for aria-label on a generic container is inconsistent. Consider role="img" so the label is reliably announced. — nitpick
• Backend contract for the pills. The pills only render when /rewards/me returns rewardTokens/rewardRecurring; normalization defaults them to null/false, so the UI degrades gracefully if the backend hasn't shipped yet. Confirm the backend actually emits these fields before un-WIP-ing, otherwise the headline feature is invisible in prod. — question (WIP)
• WIP title, ready for review. PR is isDraft: false with a WIP(...) title, so all merge gates (coverage ≥80% changed lines, i18n, lint) apply now. Either mark it draft or drop the WIP prefix once the item above is fixed. — nitpick

Looks good

• Removing the 8-cap and switching to overflow-x-auto correctly fixes the silent badge truncation; the (RewardsAchievement | null)[] typing + consistent optional-chaining is clean.
• formatTokens (compact, maximumFractionDigits: 1, Math.max(0, trunc)) matches formatNumber's locale handling and the 500K/2M/5M test expectations.
• The assignable-role ratio (unlocked && roleId, assigned counted within that set) is correct and well-commented — it can no longer report "3 of 4" for a role that can never be granted.
• i18n parity is complete: all 14 locales carry the 6 new keys and the reworded currentStreak/streakDays, with real (non-placeholder) translations and correct native scripts. pnpm i18n:check (gates only on missing/extra keys) will pass.
• Test coverage is thorough — cap removal, per-locked progress label, two-card split, days-labelled streaks, one-time vs /mo pill, and the assignable-ratio edge case are each exercised. E2E/Playwright specs updated in lockstep with the label rename.

Note: I couldn't execute the i18n scripts locally (no node_modules in this worktree), so the es/pt collision is verified by byte-comparison + reading the checker's flag logic rather than a live run — please confirm with pnpm i18n:english:check.

Requesting changes for one CI-blocking item (full review posted above).

🔴 es and pt rewards.community.rewardTokens are byte-identical to the English +{tokens} tokens, so the required pnpm i18n:english:check gate (scripts/i18n-find-english.ts, process.exit(1) on any Latin-locale value equal to en) will fail. The key isn't allowlisted — same rule you already fixed for discordDetails in this PR.

Fix (allowlist the loanword, matching discordDetails):

   "rewards.community.discordDetails", // "Discord" — brand/product name, same in every locale
+  "rewards.community.rewardTokens", // "tokens" — accepted loanword; identical in es/pt

Please run pnpm i18n:english:check to confirm green. The rest of the change (parity across all 14 locales, component logic, and test coverage) looks good — see the detailed review above for minor/optional items.

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/src/components/rewards/RewardsCommunityTab.tsx`:
- Around line 134-136: `claimingId` in `RewardsCommunityTab` is a shared scalar
and causes overlapping claim requests to overwrite each other. Change the
claim-state tracking in the claim handler/finally path to use a Set (or
equivalent per-achievement in-flight map) so multiple achievement ids can remain
pending independently, and update the button-disabled logic that currently
checks `claimingId` so it looks up the specific achievement id in that
collection. Make sure the `finally` cleanup only removes the completed id rather
than clearing all claim state.

In `@app/src/lib/i18n/ru.ts`:
- Around line 3694-3695: The reward token strings in ru.ts are inconsistent
because rewardTokens and rewardTokensMonthly still contain the English word
“tokens”; update these entries to match the existing Russian terminology used
elsewhere in the file, aligning them with claimTokens and other token-related
translations. Locate the affected locale entries by their keys
rewards.community.rewardTokens and rewards.community.rewardTokensMonthly and
replace the untranslated word so the phrasing is consistent across ru.ts.

---

Duplicate comments:
In `@app/src/services/api/rewardsApi.ts`:
- Line 154: The rewards API normalizer is forcing `claimableCount` to always be
set via `asNumber(...)`, so the optional `undefined` state from the payload is
lost. Update the `rewardsApi` summary mapping so `claimableCount` is only
assigned when `rawSummary.claimableCount` is actually present, and leave it
unset otherwise; keep the logic aligned with the `claimableCount`
typing/behavior used by the rewards types and the normalizer function handling
this summary object.

---

Nitpick comments:
In `@app/src/components/rewards/RewardsCommunityTab.tsx`:
- Around line 35-48: `claimErrorMessage` in `RewardsCommunityTab` duplicates the
same error-unwrapping flow used by `errorMessage()` in `Rewards.tsx`; extract
that shared `'error' in err` / `instanceof Error` / fallback logic into a common
helper (for example in a shared utility module) with an optional fallback
argument, then update both `claimErrorMessage` and `errorMessage()` to call it
so the behavior stays consistent in both places.

Re-review (head ebb1489, retitled feat(...), now +1115/-79)

Re-reviewed the delta since my last pass (2 new commits: feat(rewards): implement reward claiming… + refactor: … concurrent claims).

✅ Blocker resolved

rewards.community.rewardTokens is now allowlisted in scripts/i18n-find-english.ts, so pnpm i18n:english:check no longer trips on the es/pt byte-identical values. I re-verified all i18n for the 5 new claim keys (claimTokens, claiming, claimed, claimCredited, claimError): full parity across all 14 locales, real translations, native scripts present for the 6 non-Latin locales, and no Latin value collides with English. i18n is clean. 👍

New claim feature — assessment

The claiming flow is genuinely well-engineered:

• Concurrency is correct — in-flight claims tracked as a Set<id> with functional setState updates; each button disables/clears independently. The "one pending claim must not re-enable another" test proves it.
• Silent refresh (Rewards.tsx) reconciles server truth without flipping into the loading/error state, and swallows a failed background refetch so a valid page never blanks — and loadRewards(silent) never rejects, so a refresh failure can't be mis-surfaced as a claim error.
• Idempotency — the credited note is gated on !alreadyClaimed, so a re-claim doesn't imply new money.
• Server is the single source of truth for claimed/claimable; local state holds only the transient credited note + per-card error.
• Coverage is excellent — claim visibility, claim→refresh→credited, idempotent re-claim, in-flight disable, per-achievement concurrency, claimed pill, backend-error surfacing, plus API-layer claimReward/normalizeClaimResult and the silent-refresh failure path.

Nicely done. A few minor / non-blocking items:

1. formatUsd ignores the app's selected language (uses browser/OS locale) — minor

RewardsCommunityTab.tsx:29

function formatUsd(value: number): string {
  return new Intl.NumberFormat(undefined, { style: 'currency', currency: 'USD' }).format(Math.max(0, value));
}

The comment says "locale-aware USD so the money glyph matches the surrounding translated sentence," but undefined resolves to the runtime locale (browser/OS), not the app's i18n language. A user who set the app to fr on an en-US machine gets $1.25 next to French text; a de-DE machine renders 1,25 $. This also diverges from formatNumber/formatTokens, which deliberately hardcode 'en-US', and makes the '$1.25 credited…' test assertion depend on Node's default locale (fine on CI today, but implicitly coupled). Suggest either pin 'en-US' for consistency, or thread the active i18n locale in — but don't leave it as ambient undefined.

2. "Claim {tokens} tokens" when rewardTokens is null — question

RewardsCommunityTab.tsx (claim button) uses formatTokens(role.rewardTokens ?? 0), so a claimable reward with rewardTokens === null renders Claim 0 tokens. Since the claim result carries both tokens and amountUsd (and the credited note shows USD), a credit-only claimable reward seems possible. Can a claimable achievement have rewardTokens: null? If so, the 0 tokens label is misleading — consider a USD/generic fallback label (e.g. reuse claimCredited's amount or a plain "Claim reward"). If claimable always implies rewardTokens > 0, ignore this.

3. Button advertises tokens, success note shows USD — question

The button says "Claim {tokens} tokens" but the credited confirmation is {amountUsd} credited to your balance. Presumably intended (tokens = headline, USD = promo credit), just confirming the two surfaces are meant to speak different units.

Verdict

The originally blocking i18n issue is fixed and the new claiming feature is solid, correct, and thoroughly tested. Approving — the three items above are minor/optional and safe to address in a follow-up or a quick touch-up.

(As before: I couldn't run the i18n scripts live — no node_modules in this worktree — so the parity/english-check results are from byte-comparison + reading the checker logic. Worth a final pnpm i18n:check && pnpm i18n:english:check + pnpm test locally to confirm green.)